Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APIv3: return permissions expandable field on projects #10978

Merged
merged 6 commits into from
Aug 12, 2024

Conversation

humitos
Copy link
Member

@humitos humitos commented Jan 2, 2024

Add ?expand=permissions to project list/detail API endpoints.

Related readthedocs/ext-theme#128

Add `?expand=permissions` to project list/detail API endpoints.

Related readthedocs/ext-theme#128
Add `?expand=permissions` to project list/detail API endpoints.

Related readthedocs/ext-theme#128
@agjohnson agjohnson force-pushed the humitos/projects-api-permissions branch from ef4b5bc to ec87791 Compare July 1, 2024 22:17
@humitos
Copy link
Member Author

humitos commented Jul 11, 2024

@agjohnson I think we want to move away from expandable fields because it's pretty complex. So, we may not want to use this PR at all -- cc @stsewd

@agjohnson
Copy link
Contributor

What is the complexity here? This PR is very useful in removing the page load time on project/organization/build listings.

If we aren't using expand fields, what other options do we have here for returning this data in a single API request? Does this need to be a second API request from the dashboard?

@stsewd
Copy link
Member

stsewd commented Jul 11, 2024

This field is specific to the current user, so it shouldn't be a problem. But be aware that this will probably add lots of querysets when listing projects...

@agjohnson
Copy link
Contributor

Good point, I am only using this in the detail view. If there is a way to only allow this expansion on detail views, I think that should be enough as well. Or we can monitor this and see if it is ever used/abused. This should never be documented for users, though it is in a public request that users can inspect too.

@agjohnson
Copy link
Contributor

Do we need to do anything additional with this PR? This feels like a fairly small change, as are the changes to use this in

@humitos
Copy link
Member Author

humitos commented Jul 18, 2024

I'm not 100% sure about where we are regarding the expandable fields and this comment from @stsewd. I'm happy to add some test cases to this PR and mark it as ready for review after that if that's the required work here.

@agjohnson
Copy link
Contributor

@humitos I'd like to get this merged so we can get the new dashboard PR merged in too.

It sounds like this change here is not a big concern, as it's on a detail view. So seems like we just want some tests here for now?

@stsewd are you concerned about this PR at all?

@stsewd
Copy link
Member

stsewd commented Jul 31, 2024

@stsewd are you concerned about this PR at all?

Nope.

It sounds like this change here is not a big concern, as it's on a detail view. So seems like we just want some tests here for now?

+1 on tests

@humitos humitos marked this pull request as ready for review August 12, 2024 11:19
@humitos humitos requested a review from a team as a code owner August 12, 2024 11:19
Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will likely break tests on .com, make sure to update them after this PR is merged.

@humitos humitos merged commit dd557ab into main Aug 12, 2024
7 of 8 checks passed
@humitos humitos deleted the humitos/projects-api-permissions branch August 12, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants